-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix the relative path reference resolution #1131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
export function tryResolveScriptReference(program: Program, sourceFile: SourceFile, reference: FileReference) { | ||
var referenceFileName = isRootedDiskPath(reference.filename) ? reference.filename : combinePaths(getDirectoryPath(sourceFile.filename), reference.filename); | ||
referenceFileName = normalizePath(referenceFileName); | ||
if (!program.getCompilerOptions().noResolve) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why noResolve is of any consequence here. we will come here because we decided to resolve references, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just need to compute an absolute path for every file for comparison purposes regardless of --noresolve, just do not make it the main name of the file so that error messages still use the name the user inputed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhegazy don't we say if --noResolve is given do not resolve file names on the disk but use whatever is given in to the compiler, so why should we be resolving to full name in case of --noResolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--noResolve=true means do not resolve /// references. the path resolution is more of a compiler implementation detail.
I'm pleasure of use the same example I exposed in the issue but may is better use "standard" file and class names like |
…as per review feedback
--noResolve is only for ///reference and import file resolution to resolve files from disk but the file identity is always determined before creating duplicate source file for same file paths
Fix the relative path reference resolution
Compiler internally doesn't resolve the file paths to full disk paths unless necessary that results in reference resolution error when reading files that reference each other but aren't in same directory.
Eg.
src\ts\Manager\FieldManager.ts -- references ......\typings\tsd.ts
typings\tsd.ts -- reference ..\src\ts\Manager\FieldManager.ts
compiling FieldManager.ts from src\ts\Manager folder results in following reads
FieldManager.ts
......\typings\tsd.ts - as it is reference from fieldManager.ts
src\ts\Manager\FieldManager.ts as a normalized reference from tsd.ts but compiler doesn't know that this is same file as earlier read FieldManager.ts and ends up adding the text again and hence duplicate symbol errors
Fixed this by adding keeping track of disk resolved path -> SourceFile cache as well apart from just filename -> SourceFile cache when adding sourceFile to cache so if we try to lookup the same file with different relative path name, so we wouldn't end up creating multiple source files for references to the same file through different relative paths
Fixes #1039